-
Notifications
You must be signed in to change notification settings - Fork 1.2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Shopifolk can use node v20.11.1 for development #11739
Conversation
@@ -0,0 +1,54 @@ | |||
name: CI |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Decoupling the running of our lint tools (which can be run in any node version) from the running of our tests (which should only be run against the versions of node we support in package.json#engines
). See https://github.com/Shopify/polaris-internal/issues/1522 for the differences.
edb147e
to
7dcad27
Compare
@@ -26,7 +26,6 @@ | |||
"dev": "rollup -c -w", | |||
"lint": "run-p lint:*", | |||
"lint:js": "TIMING=1 eslint --cache .", | |||
"lint:styles": "stylelint '**/*.{css,scss}'", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was failing with the error:
Error: No files matching the pattern "**/*.{css,scss}" were found.
Which makes sense because there's no .css
or .scss
files in this folder.
But why was it working before?
Because the previous CI task would (unnecessarily) run yarn build
which generates dist/**/*.css
files. So, when the glob was run, there are files for stylelint
to match against, and hence no error. But those files are all in dist/
which is ignored in stylelintrc.js
, so stylelint
would silently ignore all the files and exit successfully having done nothing.
7dcad27
to
c504eef
Compare
c504eef
to
5a22920
Compare
Refs https://github.com/Shopify/polaris-internal/issues/1524 [Node 16 is EOL](https://nodejs.org/en/about/previous-releases), and [Node 18 is in Maintenance Mode](https://nodejs.org/en/about/previous-releases), so we're updating the minimum required Node version to v20.10.0 (the latest supported by `node-releases`/`browserslist` at the time this project started). ## When is node used? Node is used in 2 contexts: 1. By our consumers when they import, run, and bundle our libraries 2. By us, the Polaris team, while we develop our libraries These are very different, and have their own configs within our repository... ### 1. Consumer's node version When a consumer uses one of our libraries, they're importing whatever is in the `build/*` folder. Those assets are generated by our build process (Rollup) which transforms (via babel) whatever version of Javascript we write our code in into a version that's compatible with the [`targets` setting we provide](https://github.com/Shopify/polaris/blob/9c24a465c5e001e148bde335bf6319e924f4b1d6/polaris-react/rollup.config.mjs#L72). For example, `Array#findLast` was only made available in node v18, so when we write code like: ```javascript [1, 2, 3, 2, 1].findLast(x => x < 2); ``` The output will be different depending on our Babel config: ```javascript // with babel config: { targets: 'node 16.0' } import "core-js/modules/esnext.array.find-last.js"; [1, 2, 3, 2, 1].findLast(x => x < 2); ``` ```javascript // with babel config: { targets: 'node 18.0' } [1, 2, 3, 2, 1].findLast(x => x < 2); ``` _(try it [here](https://babeljs.io/repl#?browsers=&build=&builtIns=usage&corejs=3.21&spec=false&loose=false&code_lz=NoRgNABATJDMkwiAugOgGYEsB2ATAMgIYDOALgBQAeEAvAHwTUA80AlANxA&debug=false&forceAllTransforms=false&modules=false&shippedProposals=false&circleciRepo=&evaluate=false&fileSize=false&timeTravel=false&sourceType=module&lineWrap=true&presets=env&prettier=false&targets=Node-16.0&version=7.24.0&externalPlugins=&assumptions=%7B%7D) - set the "Env preset > Node" version to `18.0`)_ The `node 18.0` example above does not include a polyfill for `Array#findLast`, which means if our consumers try to run Polaris in a node environment (eg; during SSR) that doesn't support `Array#findLast`, they will get an error. The way we let our consumers know what is the minimum version of node they can use is via the `packages.json#engines.node` field. As long as this matches the version specified in `Babel#targets`, any consumer trying to install our library in an unsupported version of Node will receive an error. This PR updates the minimum required node version of our consumers to `20.10.0`. ### 2. Our node version while developing Polaris Similarly, the tools we use to develop Polars (Babel, Postcss, etc) may have a minimum node version requirement. They do it by specifying a `package.json#engines.node` version in their own library. Then when we install their package, if our version of node isn't compatible we'll receive an error. However, the version of node required to run Babel _has no impact on its ability to transform our code to work on older versions of node_. As long as we tell Babel which version of node to output code for in `targets`, it doesn't matter what our `.nvmrc` file says. #11739 updated the node version for developing with Polaris to `20.11.1`. --------- Co-authored-by: Aaron Casanova <[email protected]>
Use the latest version of node for local development. Closes https://github.com/Shopify/polaris-internal/issues/1525
NOTE: This does not increase the minimum required node version for consumers of our library. See https://github.com/Shopify/polaris-internal/issues/1524 for that.
I confirmed this does not impact the output of our build processes at all by running a build with node v18.12.0, then again with node v20.11.1, then diffing the generated files (there was no difference as expected).
Benefits
Performance
tl;dr: Basically no change.
Run using hyperfine: